Skip to content

ref: Make span types more robust - #10660

Merged
mydea merged 3 commits into
developfrom
fn/improve-span-types
Feb 15, 2024
Merged

ref: Make span types more robust#10660
mydea merged 3 commits into
developfrom
fn/improve-span-types

Conversation

@mydea

@mydea mydea commented Feb 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

With more strict TS settings, these could lead to problems:

  1. startChild() should return a span interface, to match the interface implementation.
  2. All properties that are optional in span/transactions need to also accept | undefined to be really correct - otherwise when we extend this and set undefined explicitly it complains there.

Fixes #10654

@mydea
mydea requested review from Lms24 and lforst February 14, 2024 16:10
@mydea mydea self-assigned this Feb 14, 2024
@github-actions

github-actions Bot commented Feb 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.82 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 69.06 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.98 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.67 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 33.25 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 33.16 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.16 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.16 KB (0%)
@sentry/browser - Webpack (gzipped) 22.43 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 76.11 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.64 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.6 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.71 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 212.88 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 101.5 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 74.01 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.83 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 69.43 KB (0%)
@sentry/react - Webpack (gzipped) 22.45 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 87.2 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 50.37 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 17.12 KB (0%)

@mydea mydea mentioned this pull request Feb 14, 2024
Closed
3 tasks done
@timfish

timfish commented Feb 15, 2024

Copy link
Copy Markdown
Collaborator

Oh wow, TS2416 is new to me 😂

If this fixes the issue, it's might be worth adding a test of some kind to ensure we don't break exactOptionalPropertyTypes anywhere else in the future!

@mydea

mydea commented Feb 15, 2024

Copy link
Copy Markdown
Member Author

TS2416

Yeah, it's a bit obscure and hard to reason about IMHO 😬 Let's see if that fixes it first, maybe we can opt-into this stricter setting in our TS E2E test 🤔

@mydea
ref: Make span types more robust
15e3745 
With more strict TS settings, these could lead to problems:

1. `startChild()` should return a span interface, to match the interface implementation.
2. no need to overwrite `updateName` for transaction, this messes with the inherited type `this` as the this is now a transaction, not a span... we can skip this.

Fixes #10654
@mydea
mydea force-pushed the fn/improve-span-types branch from 70167a8 to 15e3745 Compare February 15, 2024 08:15
@mydea

mydea commented Feb 15, 2024

Copy link
Copy Markdown
Member Author

OK, I actually tried this on the reproduction, and turns out the updateName thing was a bit misleading - it was only complaining because the transaction type was not "correct" because we had mismatches between types in interface and in the classes. Now, I made all optional properties in span/transaction accept xxx?: string | undefined, to ensure this is actually correct. The repro built for me then!

@ansf

ansf commented Feb 15, 2024

Copy link
Copy Markdown

exactOptionalPropertyTypes makes

 sampled?: boolean;

and

sampled: boolean | undefined

incompatible.

This is more in-line with the actual javascript runtime (e.g.):

const x = { sampled: undefined }
assert("sampled" in x)

Lms24
Lms24 approved these changes Feb 15, 2024
View reviewed changes

@Lms24 Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I guess this falls into the "if this is what we have to do then let's do it" category 😅

@mydea
mydea merged commit afa5304 into develop Feb 15, 2024
@mydea
mydea deleted the fn/improve-span-types branch February 15, 2024 10:37
mydea added a commit that referenced this pull request Feb 21, 2024
@mydea
ref: Make span types more robust (#10660)
01f6386 
With more strict TS settings, these could lead to problems:

1. `startChild()` should return a span interface, to match the interface
implementation.
2. All properties that are optional in span/transactions need to also
accept `| undefined` to be really correct - otherwise when we extend
this and set `undefined` explicitly it complains there.

Fixes #10654
@mydea mydea mentioned this pull request Feb 21, 2024
Merged
This was referenced Mar 12, 2024
Merged
Closed
@Lms24 Lms24 mentioned this pull request Mar 20, 2024
Merged
@Lms24 Lms24 mentioned this pull request Apr 10, 2024
Merged
This was referenced May 31, 2024
Merged
Merged
Merged
@Lms24 Lms24 mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Lms24 Lms24 Lms24 approved these changes
+1 more reviewer
@lforst lforst lforst approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mydea mydea

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

@sentry/browser version 7.93.0 and later does not build with typescript exactOptionalPropertyTypes

5 participants

@mydea @timfish @ansf @lforst @Lms24